Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Register empty validators for some MAXAR and VRICON extensions #330

Merged
merged 3 commits into from
Feb 10, 2025

Conversation

bjornblissing
Copy link
Contributor

No description provided.

Register an empty validator for MAXAR_content_3tz.
The extension does not have any properties to be validated
Register an empty validator for MAXAR_content_geojson.
The extension does not have any properties to be validated
Register empty validators for VRICON_class and VRICON_grid.
@bjornblissing bjornblissing force-pushed the MAXAR_empty_extensions branch from baab08c to b16e511 Compare January 31, 2025 09:57
@javagl
Copy link
Contributor

javagl commented Feb 4, 2025

There is a part that registers empty validators for MAXAR_content_3tz and MAXAR_content_geojson. This should not be necessary. The validators that are registered there are intended for the objects that are contained in the actual extensions dictionaries. But these extensions only allow certain content types. There are no actual extensions: { MAXAR_content_geojson: { ... } } objects for which the validation should be skipped.

(Note: Maybe the intention here was to follow the pattern of 3DTILES_content_gltf. But the difference is: In this case, there is an extensions: { 3DTILES_content_gltf: { ... } } object that may contain additional information)

But... this is related to #231 . Namely insofar that validating a tileset that declares extensionsUsed: [ "MAXAR_content_geojson" ] and actually uses GeoJSON now (still) generates two warnings

  • WARNING: Skipping 'geojson' validation
  • WARNING: The extension 'MAXAR_content_geojson' was declared in 'extensionsUsed', but not found

I have some ideas for how this could be addressed, and will open a PR for this soon.


The other empty validators are for VRICON_class and VRICON_grid. And there, the intention is to suppress the
WARNING: The extension 'VRICON_class' was used, but is not supported.

However, I saw VRICON_class, but not VRICON_grid. Is it right to assume that VRICON_grid is some sort of "legacy" version of MAXAR_grid? (If this is the case: MAXAR_grid already has an empty validator, and the approach that is implemented here would then indeed accomplish the goal of suppressing this message for VRICON_grid as well)

@bjornblissing
Copy link
Contributor Author

bjornblissing commented Feb 5, 2025

VRICON_grid is indeed the legacy version of MAXAR_grid.


Regarding MAXAR_content_geojson, it can either be used to indicate that a tile content is a .geojson file. Then there will be no object to validate, but it can also be used with a linked optional properties metadata schema for the GeoJSON content. Then there will be a extension object stored under the metadataEntity.schema.json in the tileset.
https://github.com/Maxar-Public/3d-tiles/blob/wff1.7.0/extensions/MAXAR_content_geojson/schema/metadataEntity.MAXAR_content_geojson.schema.json

Of course a better solution would be to actually validate this object as well.

@javagl
Copy link
Contributor

javagl commented Feb 5, 2025

I see, I should have checked the spec of MAXAR_content_geojson more closely.

The handling of GeoJSON in general is a bit tricky: Right now, using .geojson triggers a warning
"Skipping 'geojson' validation"

It is not clear whether an "unknown/unhandled" content type should trigger a warning (or maybe just an info?). There is a small comment in the respective code part about that, suggesting that it could make sense to make this configurable somehow.

(Note: Actually implementing a full-fledged GeoJSON validator is not in the scope of the 3D Tiles Validator. But of course, it could make sense to pull in a third-party validator for that. Quick searches suggest that there are GeoJSON validation libraries, but I have not yet investigated which one could be the most suitable here).

I have created a follow-up PR for this one at #331

If you have any preferences about how to handle .geojson (i.e. whether it should generate a warning, info, or whether that should be configurable), just let me know - maybe this could be included in that PR as well.

@javagl javagl merged commit c9c0712 into CesiumGS:main Feb 10, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants